-
Notifications
You must be signed in to change notification settings - Fork 293
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
lib/repo: Enable locking by default, make API non-experimental #1555
lib/repo: Enable locking by default, make API non-experimental #1555
Conversation
Previous discussion: #813 |
So, that seems sane to me. Though I'd like @dbnicholson to chime in if possible.
Hmm, I guess we could start annotating APIs that use the lock to try to help avoid this? |
AFAICS flatpak isn't today using the repo locking APIs. @dbnicholson Are you aware of any applications which are today? Perhaps the maximally conservative thing to do here is to keep the APIs private, and gradually extend the set of things that are safe to do concurrently with internal locking. For example today we didn't add locking to checkouts; we could do so later and document it as now safe. |
AFAIK, it's all internal. Even our flatpak fork isn't using the locking directly, but I don't think we've received any more corrupted repo reports since we enabled the locking for transactions and prunes. Sorry for not picking this up again. I'm definitely interested in completing this, but it's hard for me to find time to work on it. I do believe that ultimately you'd want to have this be public so that clients can be in control. I envision a couple ways you'd want to control the locking externally:
|
The code has been sitting around for a while but since I disabled it by default, I doubt anyone is really using it or relying on it. This patch and turns on locking by default, and also drops the API which was only public in the experimental API builds. Conceptually these are two distinct things, and we may actually want to split up the patches. I don't think this will break anyone, but it's hard to say for sure. It's also going to be hard to find out until we actually release I suspect... But anyone who is broken should be able to add `locking=false` into their repo config. On the flip side Endless has been shipping with this enabled and it is reported to help. The reason to drop the APIs: I'm a bit concerned about the interactions over time between libostree's use of the API and any apps that start using it. For example, if an app specifies a SHARED lock in their code, then later internally we decide to temporarily grab an `EXCLUSIVE`, but the app had a second thread/process that was `EXCLUSIVE` already, and that process was waiting on the first bit of code, then we could deadlock. I can't think of a real world situation where this would happen yet though. We are likely to in the future have say `fsck` take an external lock, `checkout` grab a shared one, etc.
40aed10
to
77b0ef1
Compare
I feel like concurrency cases divide into two: Those which mutate refs, and those that don't.
But that just feels weird to me. Generally a set of refs is going to be "owned" by a particular piece of software, so it could do its own locking too. For flatpak for example I don't think we'd have some non-flatpak software going in and replacing I'm not opposed to adding ostree locking for this, but would we really want flatpak/gnome-software or whatever simultaneously doing a checkout/deploy for a ref and updating it?
This one is interesting; we should drain some of this logic into libostree's fsck too. |
Anyways for now I reworked this patch again to make the APIs unconditionally private for now. We can always change that later. |
@rh-atomic-bot delegate=dbnicholson |
✌️ @dbnicholson can now approve this pull request |
I'm going to merge this. I haven't personally played much with |
The code has been sitting around for a while but since I disabled it by default, I doubt anyone is really using it or relying on it. This patch and turns on locking by default, and also drops the API which was only public in the experimental API builds. Conceptually these are two distinct things, and we may actually want to split up the patches. I don't think this will break anyone, but it's hard to say for sure. It's also going to be hard to find out until we actually release I suspect... But anyone who is broken should be able to add `locking=false` into their repo config. On the flip side Endless has been shipping with this enabled and it is reported to help. The reason to drop the APIs: I'm a bit concerned about the interactions over time between libostree's use of the API and any apps that start using it. For example, if an app specifies a SHARED lock in their code, then later internally we decide to temporarily grab an `EXCLUSIVE`, but the app had a second thread/process that was `EXCLUSIVE` already, and that process was waiting on the first bit of code, then we could deadlock. I can't think of a real world situation where this would happen yet though. We are likely to in the future have say `fsck` take an external lock, `checkout` grab a shared one, etc. Closes: #1555 Approved by: jlebon
💔 Test failed - status-atomicjenkins |
@rh-atomic-bot retry |
The code has been sitting around for a while but since I disabled it by default, I doubt anyone is really using it or relying on it. This patch and turns on locking by default, and also drops the API which was only public in the experimental API builds. Conceptually these are two distinct things, and we may actually want to split up the patches. I don't think this will break anyone, but it's hard to say for sure. It's also going to be hard to find out until we actually release I suspect... But anyone who is broken should be able to add `locking=false` into their repo config. On the flip side Endless has been shipping with this enabled and it is reported to help. The reason to drop the APIs: I'm a bit concerned about the interactions over time between libostree's use of the API and any apps that start using it. For example, if an app specifies a SHARED lock in their code, then later internally we decide to temporarily grab an `EXCLUSIVE`, but the app had a second thread/process that was `EXCLUSIVE` already, and that process was waiting on the first bit of code, then we could deadlock. I can't think of a real world situation where this would happen yet though. We are likely to in the future have say `fsck` take an external lock, `checkout` grab a shared one, etc. Closes: #1555 Approved by: jlebon
💔 Test failed - status-atomicjenkins |
@rh-atomic-bot retry |
☀️ Test successful - status-atomicjenkins |
Since ostreedev/ostree#1555, locking is enabled by default in OSTree. Unfortunately it uses thread-private data and it breaks the Golang bindings. Force the same thread for the write operations to the OSTree repository. Signed-off-by: Giuseppe Scrivano <[email protected]>
Needed to pick up this change: ostree: use the same thread for ostree operations Since ostreedev/ostree#1555, locking is enabled by default in OSTree. Unfortunately it uses thread-private data and it breaks the Golang bindings. Force the same thread for the write operations to the OSTree repository. Signed-off-by: Giuseppe Scrivano <[email protected]>
Needed to pick up this change: ostree: use the same thread for ostree operations Since ostreedev/ostree#1555, locking is enabled by default in OSTree. Unfortunately it uses thread-private data and it breaks the Golang bindings. Force the same thread for the write operations to the OSTree repository. Signed-off-by: Giuseppe Scrivano <[email protected]>
Since ostreedev/ostree#1555, locking is enabled by default in OSTree. Unfortunately it uses thread-private data and it breaks the Golang bindings. Force the same thread for the write operations to the OSTree repository. Signed-off-by: Giuseppe Scrivano <[email protected]>
Since ostreedev/ostree#1555, locking is enabled by default in OSTree. Unfortunately it uses thread-private data and it breaks the Golang bindings. Force the same thread for the write operations to the OSTree repository. Signed-off-by: Giuseppe Scrivano <[email protected]>
Since ostreedev/ostree#1555, locking is enabled by default in OSTree. Unfortunately it uses thread-private data and it breaks the Golang bindings. Force the same thread for the write operations to the OSTree repository. Signed-off-by: Giuseppe Scrivano <[email protected]>
Since ostreedev/ostree#1555, locking is enabled by default in OSTree. Unfortunately it uses thread-private data and it breaks the Golang bindings. Force the same thread for the write operations to the OSTree repository. Signed-off-by: Giuseppe Scrivano <[email protected]>
Since ostreedev/ostree#1555, locking is enabled by default in OSTree. Unfortunately it uses thread-private data and it breaks the Golang bindings. Force the same thread for the write operations to the OSTree repository. Signed-off-by: Giuseppe Scrivano <[email protected]>
Since ostreedev/ostree#1555, locking is enabled by default in OSTree. Unfortunately it uses thread-private data and it breaks the Golang bindings. Force the same thread for the write operations to the OSTree repository. Signed-off-by: Giuseppe Scrivano <[email protected]>
Since ostreedev/ostree#1555, locking is enabled by default in OSTree. Unfortunately it uses thread-private data and it breaks the Golang bindings. Force the same thread for the write operations to the OSTree repository. Signed-off-by: Giuseppe Scrivano <[email protected]>
Since ostreedev/ostree#1555, locking is enabled by default in OSTree. Unfortunately it uses thread-private data and it breaks the Golang bindings. Force the same thread for the write operations to the OSTree repository. Signed-off-by: Giuseppe Scrivano <[email protected]>
Since ostreedev/ostree#1555, locking is enabled by default in OSTree. Unfortunately it uses thread-private data and it breaks the Golang bindings. Force the same thread for the write operations to the OSTree repository. Signed-off-by: Giuseppe Scrivano <[email protected]>
The code has been sitting around for a while but since I disabled
it by default, I doubt anyone is really using it or relying on it.
This patch makes the existing APIs public, and turns on locking
by default. Conceptually these are two distinct things, and we
may actually want to split up the patches.
I don't think this will break anyone, but it's hard to say for sure.
It's also going to be hard to find out until we actually release
I suspect...
But anyone who is broken should be able to add
locking=false
intotheir repo config.
On the APIs - I'm a bit concerned about the interactions over time
between libostree's use of the API and any apps that start using it.
For example, if an app specifies a SHARED lock in their code, then
later internally we decide to temporarily grab an
EXCLUSIVE
, but theapp had a second thread/process that was
EXCLUSIVE
already, andthat process was waiting on the first bit of code, then we could
deadlock. I can't think of a real world situation where this would happen
yet though.